-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[BUGFIX] Inconsistent naming of [cross_]axis_tilt / [cross_]axis_slope
#2543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[BUGFIX] Inconsistent naming of [cross_]axis_tilt / [cross_]axis_slope
#2543
Conversation
|
Currently targets v0.13.1, but I leave this decision up to anybody else (please, assign milestone accordingly). I will update deprecations tracker when merged. |
pvlib/shading.py
Outdated
| since="0.13.1", | ||
| old_param_name="cross_axis_slope", | ||
| new_param_name="cross_axis_tilt", | ||
| removal="0.15.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| removal="0.15.0", |
We usually don't specify a removal version as there's not standard timeline for when minor versions come out. If we really wanted to specify a removal, I think this should be in the form of a date, e.g., removal="Earliest September 2026".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never thought of that, but looks a promising idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm interested in what @wholmgren thinks of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this for a few weeks. I don't think timeline is particularly relevant so long as we are using SemVer (or something close to it). I personally think we should specify removal versions, they should default to the 0.(m+2).0 release, and we should use our existing test machinery to enforce that. If we had several minor releases in a very short period of time (a few weeks) then I'd be ok pushing out the removal to a future release.
|
The original thread #2334 contained multiple views in favor of |
|
Thanks for the reviews. @AdamRJensen , yeah, my fault. This is a stale branch I had locally - I guess I took that decision to conform with what already was the term prior to the newer feature and just skimmed through the discussion. I will change this whenever I can to reflect what was discussed. |
Co-Authored-By: RDaxini <[email protected]>
|
After a deeper review on the required changes to port
But some other occurrences that could come along with the same renaming:
The other way around is what is currently implemented in this PR,
I personally would prefer coherence among all of them. Either tilts or slopes. An example of what I mean, if I only address 1., Note 1. and 5. are solvable with the renamed kwarg warning util; 2. and 3. by copying the new signature into an old reference variable, prior decoration with the deprecated util. À la: # allow deprecated name from calc_cross_axis_slope
# (original name was calc_cross_axis_tilt)
calc_cross_axis_tilt = deprecated(
since="0.13.1", alternative="pvlib.tracking.calc_cross_axis_slope"
)(calc_cross_axis_slope)Regarding 4., I have my doubts. Thou probably populating and overriding its To sum, my points: a. To which extent do we want to rename My two opinionated cents: Nonetheless, a weak opinion. Please leave your opinion and suggestions. I will do as requested. CC: @pvlib/pvlib-maintainer |
|
I believe we selected “tilt” versus slope because we wanted to distinguish between the “tilt” of the plane containing tracker axes versus the slope of the underlying terrain which might differ and is not relevant to the tracker rotation calculation. IMO slope has a connotation that implies terrain. |
|
I added a comment in the issue, #2334 (reply in thread), but I'll duplicate it here: I'll caveat this with noting that I don't think it matters too much, but I still prefer "slope." For example, I can mention "slope-aware backtracking," and it is relatively unambiguous what I am talking about. If I were instead to mention "tilt-aware backtracking," I think the meaning is lost. And I informally asked a few non-solar folks in the office about the image below. Everyone said the angle of B should be "tilt" and C should be "slope" or "incline." When I said B has to be "tilt" and asked if A should be "tilt" or "slope", everyone liked "slope" more and said that "tilt" would be confusing. One person did mention that slope makes them think of "rise over run"...
|
|
Hella lot of changes in the new diff. My condolences to reviewers. |
|
@echedey-ls fyi -- it seems like this change needs more discussion and review before it's ready for merge, so I moved the milestone to v0.13.2. |
|
No problem! |
cross_axis_tilt / cross_axis_slope[cross_]axis_tilt / [cross_]axis_slope
|
Adding an illustration to the docstring of |
Agreed. I suggest a new issue/PR for that. IMO, diff is already big in this PR + personally I don't have time to contribute now. |
1 similar comment
Agreed. I suggest a new issue/PR for that. IMO, diff is already big in this PR + personally I don't have time to contribute now. |
|
Are there existing examples of illustrations in docstrings? https://pvlib-python.readthedocs.io/en/latest/reference/generated/pvlib.shading.shaded_fraction1d.html is the only one I know of. A collection of diagrams/illustrations for pvlib with common design themes/standards and a straightforward way to update them would be really cool. I know I've talked with @kandersolar and @mikofski about tools to create diagrams (other than PowerPoint, my unfortunate go-to), but I can't remember what they use. |
| A value denoting the compass direction along which the axis of | ||
| rotation lies. Measured in decimal degrees east of north. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| The compass direction along which the axis of | |
| rotation lies, in decimal degrees east of north. |
| west. Use :func:`~pvlib.tracking.calc_cross_axis_tilt` to calculate | ||
| ``cross_axis_tilt``. [degrees] | ||
| cross_axis_slope : float, default 0.0 | ||
| Angle of the plane containing the rows' axes relative to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct. It describes an angle "relative to horizontal", i.e., an angle in the earth-centered reference frame.
cross_axis_slope is axis_slope=90, cross_axis_slope is an angle within the horizontal (earth-centered) plane. @kandersolar is this correct?
The old definition of cross_axis_tilt has a similar problem.
I think cross_axis_slope has to somehow be stated relative to the tracker coordinates, i.e., the angle formed by a line perpendicular to two tracker rotation axes, and the tracker x-axis.

docs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor all changes. Includes link to the GitHub Issue with:issue:`num`or this Pull Request with:pull:`num`. Includes contributor name and/or GitHub username (link with:ghuser:`user`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.Extract from the body message, OP @williamhobbs, #2334 .
Also adds variable description to nomenclature page.
Relevant doc pages (old)https://pvlib-python--2543.org.readthedocs.build/en/2543/user_guide/extras/nomenclature.html#term-cross_axis_tilthttps://pvlib-python--2543.org.readthedocs.build/en/2543/reference/generated/pvlib.shading.shaded_fraction1d.htmlhttps://pvlib-python--2543.org.readthedocs.build/en/2543/reference/generated/pvlib.tracking.singleaxis.htmlRelevant doc pages (new)